-
Notifications
You must be signed in to change notification settings - Fork 13.1k
chore(apps): refactor jsonrpc request handlers parameter passing #38322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughRefactors Deno runtime handlers to accept a consolidated Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38322 +/- ##
===========================================
+ Coverage 70.76% 70.77% +0.01%
===========================================
Files 3159 3159
Lines 109397 109397
Branches 19657 19675 +18
===========================================
+ Hits 77416 77431 +15
+ Misses 29950 29934 -16
- Partials 2031 2032 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
05a53ac to
8d51b92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 27 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/apps-engine/deno-runtime/handlers/app/handleOnPreSettingUpdate.ts (1)
17-23: Validate params length and first element shape before destructuring.
Array.isArray(params)allows[], which yieldssetting === undefinedand can surface as a harder-to-debug runtime error inside the app handler. Consider guarding for length and a non-null object before calling the hook.🔧 Suggested fix
- if (!Array.isArray(params)) { + if (!Array.isArray(params) || params.length < 1 || typeof params[0] !== 'object' || params[0] === null) { throw new Error('Invalid params', { cause: 'invalid_param_type' }); } - const [setting] = params as [Record<string, unknown>]; + const [setting] = params as [Record<string, unknown>];packages/apps-engine/deno-runtime/handlers/app/handleOnSettingUpdated.ts (1)
17-21: Missing validation for empty params array.If
paramsis an empty array,settingwill beundefinedafter destructuring, andapp.onSettingUpdatedwill be called with an undefined setting. Consider validating that the setting exists.Proposed fix
if (!Array.isArray(params)) { throw new Error('Invalid params', { cause: 'invalid_param_type' }); } const [setting] = params as [Record<string, unknown>]; +if (!setting) { + throw new Error('Setting is required', { cause: 'invalid_param_type' }); +} + await app.onSettingUpdated(setting, AppAccessorsInstance.getConfigurationModify(), AppAccessorsInstance.getReader(), AppAccessorsInstance.getHttp());packages/apps-engine/deno-runtime/handlers/outboundcomms-handler.ts (1)
8-18: Add validation forparamsshape before spreading.
In JSON-RPC 2.0,paramscan be either an Array (positional) or an Object (named). The current code castsparamstoArray<unknown>without validation, which silently fails whenparamsis an object—the spread operator produces an empty array, causing the provider method to receive incorrect arguments. Add a guard to validateparamsis array-shaped and return a controlled error for invalid shapes, consistent with patterns already used inscheduler-handler.tsandconstruct.ts.Proposed fix
export default async function outboundMessageHandler(request: RequestObject): Promise<JsonRpcError | Defined> { const { method: call, params } = request; const [, providerName, methodName] = call.split(':'); + if (params !== undefined && !Array.isArray(params)) { + return new JsonRpcError('error-invalid-params', -32000); + } const provider = AppObjectRegistry.get<IOutboundMessageProviders>(`outboundCommunication:${providerName}`); if (!provider) { return new JsonRpcError('error-invalid-provider', -32000); } const method = provider[methodName as keyof IOutboundMessageProviders]; const logger = AppObjectRegistry.get<Logger>('logger'); const args = (params as Array<unknown>) ?? [];packages/apps-engine/deno-runtime/handlers/videoconference-handler.ts (1)
27-27: Consider handling undefinedparams.In
jsonrpc-lite,RequestObject.paramsis optional. If a request arrives without params, this line will throw when attempting to destructureundefined. Consider defaulting to an empty array.Suggested fix
- const [videoconf, user, options] = params as Array<unknown>; + const [videoconf, user, options] = (params ?? []) as Array<unknown>;packages/apps-engine/deno-runtime/handlers/api-handler.ts (1)
2-32: Validate params shape before destructuring.If
paramsis missing or not an array,requestData/endpointInfobecomeundefinedand the endpoint call can fail with an internal error instead of a JSON‑RPC invalid‑params response. Other handlers in this codebase validate this consistently.🔧 Proposed fix
- const [requestData, endpointInfo] = params as Array<unknown>; + if (!Array.isArray(params) || params.length < 2) { + return JsonRpcError.invalidParams(null); + } + const [requestData, endpointInfo] = params as [unknown, unknown];
🤖 Fix all issues with AI agents
In `@packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts`:
- Around line 28-33: The destructuring of params happens outside the try and can
throw if params is missing or malformed; move parsing of params into the try
block inside handleUploadEvents, validate that request.params is an array and
has at least one element (and that the first element is an object) before doing
const [{ file, path }] = params, and if validation fails return a JsonRpcError
or appropriate error object; keep the rawMethod -> method parsing (rawMethod and
method derived from uploadEvents) but perform the params checks first to ensure
safe destructuring and proper JSON-RPC error flow.
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts
Outdated
Show resolved
Hide resolved
1ca4ed8 to
7307eb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/apps-engine/deno-runtime/handlers/api-handler.ts (1)
8-27: Add params validation before destructuring to prevent uncaught exceptions.Line 25 destructures
paramswithout guarding againstundefined,null, or non-array values. Ifparamsis invalid, destructuring throws and bypasses error handling. Add an early Array.isArray check, consistent withscheduler-handler.tsandlistener/handler.ts:Suggested fix
export default async function apiHandler(request: RequestObject): Promise<JsonRpcError | Defined> { const { method: call, params } = request; const [, path, httpMethod] = call.split(':'); const endpoint = AppObjectRegistry.get<IApiEndpoint>(`api:${path}`); const logger = AppObjectRegistry.get<Logger>('logger'); if (!endpoint) { return new JsonRpcError(`Endpoint ${path} not found`, -32000); } const method = endpoint[httpMethod as keyof IApiEndpoint]; if (typeof method !== 'function') { return new JsonRpcError(`${path}'s ${httpMethod} not exists`, -32000); } + if (!Array.isArray(params)) { + return JsonRpcError.invalidParams(null); + } + const [requestData, endpointInfo] = params as Array<unknown>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 28 files
585160e to
1090894
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/apps-engine/deno-runtime/handlers/api-handler.ts (1)
26-41: Guard against non‑array params before destructuring.If
paramsis not an array, the destructuring throws and is reported as an internal error. Consider validating and returning a JSON‑RPC invalid‑params error instead.🛠️ Suggested fix
- const [requestData, endpointInfo] = params as Array<unknown>; + if (!Array.isArray(params) || params.length < 1) { + return new JsonRpcError('Invalid params', -32602); + } + const [requestData, endpointInfo] = params as Array<unknown>;packages/apps-engine/deno-runtime/handlers/tests/uikit-handler.test.ts (1)
4-103: Tests passRequestObjectbut handler expectsRequestContextwith logger.The
handleUIKitInteractionfunction explicitly requiresRequestContext(which includescontext.logger), but the tests pass plainRequestObjectinstances fromjsonrpc.request(). While the handler doesn't currently use the logger field, this violates the type contract and should be fixed for type safety.Wrap request objects with the required context:
🛠️ Suggested fix
import { afterAll, beforeEach, describe, it } from 'https://deno.land/[email protected]/testing/bdd.ts'; import jsonrpc from 'jsonrpc-lite'; +import { Logger } from '../../lib/logger.ts'; import { AppObjectRegistry } from '../../AppObjectRegistry.ts'; import handleUIKitInteraction, { UIKitActionButtonInteractionContext, UIKitBlockInteractionContext, UIKitLivechatBlockInteractionContext, UIKitViewCloseInteractionContext, UIKitViewSubmitInteractionContext, } from '../uikit/handler.ts'; +const makeRequest = (id: number, method: string, params: unknown[]) => + Object.assign(jsonrpc.request(id, method, params), { + context: { logger: new Logger(method) }, + }); describe('handlers > uikit', () => { const mockApp = { getID: (): string => 'appId', executeBlockActionHandler: (context: any): Promise<any> => Promise.resolve(context), executeViewSubmitHandler: (context: any): Promise<any> => Promise.resolve(context), executeViewClosedHandler: (context: any): Promise<any> => Promise.resolve(context), executeActionButtonHandler: (context: any): Promise<any> => Promise.resolve(context), executeLivechatBlockActionHandler: (context: any): Promise<any> => Promise.resolve(context), }; beforeEach(() => { AppObjectRegistry.set('app', mockApp); }); afterAll(() => { AppObjectRegistry.clear(); }); it('successfully handles a call for "executeBlockActionHandler"', async () => { - const request = jsonrpc.request(1, 'app:executeBlockActionHandler', [ + const request = makeRequest(1, 'app:executeBlockActionHandler', [ { actionId: 'actionId', blockId: 'blockId', value: 'value', viewId: 'viewId', }, ]); const result = await handleUIKitInteraction(request); assertInstanceOf(result, UIKitBlockInteractionContext); }); it('successfully handles a call for "executeViewSubmitHandler"', async () => { - const request = jsonrpc.request(1, 'app:executeViewSubmitHandler', [ + const request = makeRequest(1, 'app:executeViewSubmitHandler', [ { viewId: 'viewId', appId: 'appId', userId: 'userId', isAppUser: true, values: {}, }, ]); const result = await handleUIKitInteraction(request); assertInstanceOf(result, UIKitViewSubmitInteractionContext); }); it('successfully handles a call for "executeViewClosedHandler"', async () => { - const request = jsonrpc.request(1, 'app:executeViewClosedHandler', [ + const request = makeRequest(1, 'app:executeViewClosedHandler', [ { viewId: 'viewId', appId: 'appId', userId: 'userId', isAppUser: true, }, ]); const result = await handleUIKitInteraction(request); assertInstanceOf(result, UIKitViewCloseInteractionContext); }); it('successfully handles a call for "executeActionButtonHandler"', async () => { - const request = jsonrpc.request(1, 'app:executeActionButtonHandler', [ + const request = makeRequest(1, 'app:executeActionButtonHandler', [ { actionId: 'actionId', appId: 'appId', userId: 'userId', isAppUser: true, }, ]); const result = await handleUIKitInteraction(request); assertInstanceOf(result, UIKitActionButtonInteractionContext); }); it('successfully handles a call for "executeLivechatBlockActionHandler"', async () => { - const request = jsonrpc.request(1, 'app:executeLivechatBlockActionHandler', [ + const request = makeRequest(1, 'app:executeLivechatBlockActionHandler', [ { actionId: 'actionId', appId: 'appId', userId: 'userId', visitor: {}, isAppUser: true, room: {}, }, ]); const result = await handleUIKitInteraction(request); assertInstanceOf(result, UIKitLivechatBlockInteractionContext); }); });
🧹 Nitpick comments (2)
packages/apps-engine/deno-runtime/handlers/tests/api-handler.test.ts (1)
33-74: Preserve full RequestContext shape in test requests.
apiHandlernow expects aRequestContextwithcontext.logger. The request objects built here omitcontext, which can break type-checking and drift from runtime shape. Consider adding a minimalcontextto the request payloads.♻️ Proposed fix
- const result = await apiHandler(jsonrpc.request(1, 'api:/test:get', ['request', 'endpointInfo'])); + const result = await apiHandler({ ...jsonrpc.request(1, 'api:/test:get', ['request', 'endpointInfo']), context: { logger: undefined as any } }); @@ - const result = await apiHandler(jsonrpc.request(1, 'api:/test:post', ['request', 'endpointInfo'])); + const result = await apiHandler({ ...jsonrpc.request(1, 'api:/test:post', ['request', 'endpointInfo']), context: { logger: undefined as any } }); @@ - const result = await apiHandler(jsonrpc.request(1, `api:/test:delete`, ['request', 'endpointInfo'])); + const result = await apiHandler({ ...jsonrpc.request(1, `api:/test:delete`, ['request', 'endpointInfo']), context: { logger: undefined as any } }); @@ - const result = await apiHandler(jsonrpc.request(1, `api:/error:get`, ['request', 'endpointInfo'])); + const result = await apiHandler({ ...jsonrpc.request(1, `api:/error:get`, ['request', 'endpointInfo']), context: { logger: undefined as any } }); @@ - const result = await apiHandler(jsonrpc.request(1, `api:/test:put`, ['request', 'endpointInfo'])); + const result = await apiHandler({ ...jsonrpc.request(1, `api:/test:put`, ['request', 'endpointInfo']), context: { logger: undefined as any } });packages/apps-engine/deno-runtime/main.ts (1)
57-83: Consider avoiding in-place mutation when buildingRequestContext.
Object.assign(payload, …)mutates the original payload. A spread keeps the request immutable and avoids unexpected side effects if the payload is reused later.♻️ Suggested change
- const context: RequestContext = Object.assign(payload, { - context: { logger } - }) + const context: RequestContext = { + ...payload, + context: { logger }, + };
Proposed changes (including videos or screenshots)
This refactor allows each handler to have access to the request object reference and consequently its ID. This is an enabler to fix an issue with logs in "nested" requests.
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Refactor
Style / Errors
Tests
✏️ Tip: You can customize this high-level summary in your review settings.